Skip to content
This repository has been archived by the owner on Jun 19, 2023. It is now read-only.

add race fix for HashOnRead #50

Merged
merged 1 commit into from
Apr 13, 2020
Merged

add race fix for HashOnRead #50

merged 1 commit into from
Apr 13, 2020

Conversation

bonedaddy
Copy link
Contributor

fixes #49

@Stebalien
Copy link
Member

Yep, that's a bad API. What do you think about #51? It's breaking, but IMO, makes a lot more sense than this API.

@bonedaddy
Copy link
Contributor Author

Personally I like this API since it allows you to toggle verification functionality at runtime without needing to teardown existing applications + restart just to get the verification functionality. There are benefits to the change proposed in #51, however it will require projects to deal with the breaking change, which isn't necessarily the easiest thing depending on how heavily used HashOnRead is by some other projects.

It might make sense to implement both fixes, so projects can pull in the race condition fix without needing to deal with breaking changes until they're ready.

@Stebalien Stebalien merged commit 8549146 into ipfs:master Apr 13, 2020
@Stebalien
Copy link
Member

I'm merging this for now given that it does fix the bug. However, I'm still uncomfortable with the API.

Implementations: https://grep.app/search?q=func.%2A%20HashOnRead%5C%28&regexp=true&case=true

Other than this implementation:

  • Two panic (not implemented).
  • One does nothing.
  • All others are wrappers that forward to the underlying blockstore.

That really smells like a bad API.

@hsanjuan
Copy link
Collaborator

If this is meant for 0.5.0 it needs to be backported to a blockstore 0.1.x branch. (I can do, let me know).

@Stebalien
Copy link
Member

This doesn't need to go into the release (we never call this after initializing the blockstore.

@aschmahmann aschmahmann mentioned this pull request Dec 13, 2021
59 tasks
Jorropo pushed a commit to ipfs/go-libipfs-rapide that referenced this pull request Mar 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HashOnRead Has A Race Condition
3 participants